[ALSA] Improve stream setup parameters. #582
Conversation
* Always set period before setting buffer size, even if `BufferSize::Fixed`. * Set `start_threshold` based on `alsa::Direction` to avoid underruns on playback streams. Also refactor the return values of the set params functions to reflect their actual use and resolve some clippy warnings (including a logic error in `stream_timestamp()`).
|
Thanks for opening this one. I see the WASM and clippy-test build failures are already present on master, hopefully unrelated? |
|
@strohel yeah I'll merge it. PRs to fix the CI build welcome, even if it disables the jobs instead of fixing them. |
| BufferSize::Fixed(v) => hw_params.set_buffer_size(v as alsa::pcm::Frames)?, | ||
| BufferSize::Fixed(v) => { | ||
| hw_params.set_period_size_near((v / 4) as alsa::pcm::Frames, alsa::ValueOr::Nearest)?; | ||
| hw_params.set_buffer_size(v as alsa::pcm::Frames)?; | ||
| } |
There was a problem hiding this comment.
Summary: I think this causes a regression when using odd buffer sizes. It's not a big deal for me because I can just choose a more reasonable size, but I thought you might wanna know.
I just updated cpal in a project of mine from 0.13.3 to 0.13.4 and this caused a regression. With 0.13.3 everything worked, but with 0.13.4 I get this error:
A backend-specific error has occurred: ALSA function 'snd_pcm_hw_params_set_buffer_size' failed with error 'EINVAL: Invalid argument'
- Ubuntu 20.04
SupportedStreamConfig::buffer_sizeisRange { min: 3, max: 4194304 }StreamConfig { channels: 2, sample_rate: SampleRate(44100), buffer_size: Fixed(735) }- f32 samples
The problem here is the weird buffer size. Changing that to 1024 (for example) fixes the error and everything works again.
I'm commenting on this line change because I think this caused the regression (seems to be the only ALSA related change between the two versions).
Unfortunately, I don't have the time to provide a minimal example. In case you want to try with the project I mentioned:
Details
this project at 61990b4ccab688a62172ef7836ea15b60f1cc942. The commit works, but try updating cpal. To actually run this, you have to run testroms/download.sh and then cargo run -- testroms/blargg/dmg_sound/dmg_sound.gb.
There was a problem hiding this comment.
Hmm I'm trying to think of examples where you need odd buffer sizes. Maybe it would be good to forbid their use? Maybe it should be documented then.
There was a problem hiding this comment.
Sounds like a solution to me. I also cannot come up with a use case for that -- even numbers should suffice. (Tho I'm not really experienced with audio...). But yeah, that would certainly be better than a non-descriptive "backend-specific error".
4 participants
Rebased by @strohel . Rebased version of #520.
BufferSize::Fixed.start_thresholdbased onalsa::Directionto avoid underruns onplayback streams.
Also refactor the return values of the set params functions to reflect their
actual use and resolve some clippy warnings (including a logic error in
stream_timestamp()).